I like the "Clean Code" idea of a method only doing one thing, but in practice it's really annoying to jump around a file, or multiple files, trying to figure out how it all fits together, and identifying the hidden side effects. I think using extract method to create pure functions (ones that don't have side effects, and make use of immutable/read-only data types) is probably a good use-case or guideline to follow for private class methods.
Also, the indirection often means the code's doing unnecessary stuff, like loading the same data from a database twice (I've seen this happen many times).
It always depends. Indirection can be useful for maintenance if done right. Throwing everything in there because "it's easier", makes the code hard to maintain.
@@danflemming3553 I second this. Having a regular sized code base with duplicated code all over the place just to "avoid indirections" is a pain in the ass to maintain. Have a reusable method instead and make your change in only one place
As with anything, there are some developers who really don't grasp a code concept. You have programmers who happily code a function/method of hundreds of lines of code with global variables/mutables sprinkled all over the place. Others turn 20 lines of code into 60 separate libraries.
I use Extract Method primarily to not to have to write comments. By introducing another method you have an opportunity to give it a meaning full name. That is your 'comment'. A long method can read like a book, so length is not the primary issue. Of course this is all highly subjective.
Agree. One of the issues with the "50 lines" version is every developer that has to understand what the method essentially has to reverse engineer the method by walking through it the way he did in the video. I don't want to have to read the logic (how it works) to understand the broad concepts (what it does). A section of code will be read many more times than it will be written or modified. Extracted methods save developers time (if done well). Side note, as for the Validate methods that throw, you could name them, e.g., ThrowIfInvalidRequest, etc. to make it obvious.
I was worried by the title, but in the end, I fully agree with your explanations. The problem is not the "Extract Method" refactoring, the problem is extracting the wrong set of lines. E.g. checking a condition and then throwing an exception can already be counted as two separate things, even disregarding that the exception changes the control flow. The condition check could be a useful part of the domain model for reuse elsewhere. The decision what to do based on the outcome could be different in different scenarios. Deep in the code, an exception might be correct, but at the HTTP API level, producing an HTTP 4xx response is probably the better option.
Depends a bit on the complexity of the extracted function, if needed to be extracted anyway. 5 lines 10 line 100 lines,.. throwIfInvalid(user); gives a fast insight and sums up what it does, no matter how complex. As it not only validates the user, it throws, as part of what it does as well. This reads just as clear as "if xxz then throw". But as always, it allways depends! :-)
Yeah, naming is key. If the primary purpose of the method is to cause an exception if some condition fails, maybe include that in the name! Or maybe move your exceptions themselves into another object, move the condition into a bool function, and just do `if (conditionMethod()) throw errors.nameOfError;` or something like that. You don't necessarily need to extract the entire logic of what you are doing, but you can make the code a lot cleaner and more manageable by putting the complicated or verbose portions someplace else.
It is also a common pattern in Java to use void functions that start with "validate" and it is very clear (even though it's still only implied) that it throws if there's a validation error. eg. void validateJwt(String token)
It is all about context. This method is involved with creating a user. The moment channels becomes the main subject, the code goes into a new method. The existing user could have stayed contextwise, but readability is another factor to consider.
I like to abstract even further by creating smaller "classes" to handle the tasks like "Validation", "Database Operations" and other logic. Extract Method still has a good place, like in the video's example, where it makes complex conditional statements extremely read-able in the actual method itself. Additionally, the smaller classes make it real easy to test that code in isolation from the rest of the context and logic of the program.
Yeah. I believe any kind of Refactoring should be considered as a trade-off rather than a value-added action. That's why Refactoring techniques usually come in pair, so that when we find one is becoming too much, we can re-calibrate the trade-off with the opposite one (which is the later part of your video). For the case "Extract Method", its pair would be "Inline Method." But the problem we have at the moment is most IDEs offer the automatic "Extract Method" refactoring, but not the automatic "Inline Method" refactoring. So it becomes too easy to extract methods automatically, but only a few who properly learn the craft from Martin Fowler and Kent Beck's book can safely inline them back manually. And the way IDEs only offer automatic "Extract Method" may also give many developers the wrong impression that "Extract Method" is a good thing and should be encouraged. So... we have "Extract Method" happening and spreading like cancer.
50 lines with little nesting? Very fine in my books, usually. Hundreds of lines, thousands? With deep nesting? I dunno how many such methods I've encountered , a lot certainly, "Extract Method" was the first shortcut command I learned in VS. Just the other day I was working on a class that had the same calculation duplicated 19 times in one method. Usually I will only have to do "Inline Method" in my own code where I use "Extract Method" as a way of exploration.
I think the Validate method is solved by having conventions. For example we use requireXX. When reading the require prefix and by being (usually) at top of the method I think it's pretty okay to know it throws. Ofc here the validations are trivial and don't really need to be extracted anyway as you pointed out. Other than this I agree 💯%.
Primarily, extracting code blocks are actually very useful in 2 scenarios for me 1. When it becomes easy for me to lose track of the execution sequence of a code. I prefer to group each block by functionality. That way, I'm able to follow my logic while not worrying about the implementation of the high level lines of code 2. When a block of code is being reused across multiple implementations.
Are you supposed to read the code and learn everything how it works? I don't think so. If you need to learn how everything works because there's a bug which is hard to pinpoint, that kind of bug is hard to fix if the code is bad even without all the indirection by different methods. In your example, bad design like the FilterAgeRestrictedChannels which does not follow CQS, is different and not related to the indirection. Bad indirection does exist, but I don't think your example was a good one. The ValidateUsername and ValidateExistingUser were OK, when I want to extend the code in the Handle(), I want to understand it quickly.
@@Ry4nWTF CQS - command query segregation, meaning a method should either return a result (query) or perform state changes (command), but not both. Bascially, you should avoid modifying your input params. In the `FilterAgeRestrictedChannels` method, he was modifying the `channel` input with the `addRange` call. Hence, breaking the CQS principle. This is a really bad code smell in my book, as I have been bitten by bugs introduced by such practices in the past, and it's really hard to debug. This is also why FP fellas mock OOP (mutable states all over the place), and what functional features like LINQ are good at solving. (Dang I miss LINQ so much. Java Streams are so verbose...)
A method should have one level of abstraction. If the method has one level of abstraction then it is okay that it is contain many lines because it's is easy to read.
I would say that as a rule of thumb for non-public code: Only extract when you need to hide parts of the implementation, or rather behavior, behind a name that makes sense. Developer are bad at doing in consistently when only want to finishing things. This might cause more convoluted code with names not indicating the behavior. So they think reviews will catch those, not necessarily since people are looking more at style. Since no one discusses the actual design, in my experience.
I totally agree with this. It's annoying when people want to extract method just because something is long, and then end up passing a million parameters and naming the methods with nonsensical names. Btw, I wouldn't call this "indirection"; I use that word for calling abstract methods (which also really hurts readability in general).
While also being kind of an abstraction, I like to define the conditionals as variables if I want to give them a name, that way the logic stays in scope
That is the way to work with code, loved watching it. Basically the attempt to extract meaningful code blocks from large methods is very valuable. But yes, if not done carefully, it can increase the "spaghettiness" a lot, while actually wanting to improve the code. It is worth reasoning about what readability means for that code at hand. It is not the formatting alone of course. But even when the "good" refactorings are not easy to spot, it is important to keep doing this continuously, even when the code seems already finished. The more skilled at refactoring we are, the better code quality becomes, both of existing and of new code. I personally think that refactoring is the most underrated but most important activity in coding. And we see how important naming is. And immutability, or that a method should either check state or change it, but never do both at the same time. Sometimes I find myself inlining short methods back in. And then sometimes it makes sense to extract a class and/or interface. We should not overlook that option, while this one must be handled with even a lot more care 🫣😄 Looking forward to that video about exceptions versus return values / control flow! 😃 Great stuff 👍
This seems like the problem is doing side effects in methods and not doing extract method. If your method was profile.Channels = RestrictChannels(profile.Channels) then it would have been very clear. Same with the result type which you already mentioned. An extra layer of indirection is fine if it helps you read the code at a glance IMO. It can be taken too far, but code that reads like natural language is very easy to figure out what it is (or should be) doing. Your tests will hopefully make sure that the actual implementation is fine. Like you said, it depends on the context, but I don't hate the advice of trying to stick to one layer of abstraction per method.
Absolutely it's side-effects as well as levels of indirection. My example wasn't so bad but I think we've all read or created without realizing it some absurd levels of indirection
I didn't even know Extract Method was even a thing. I always manually do it, and I usually avoid mutating parameters where I can. Validation methods that throw errors or just return usually have a naming convention that I follow, but admittedly, it's probably better to just return a [error, result] type. If only I could do that in Java a lot easier without tons of boilerplate. I don't know if a good solution has emerged beyond Java 8. In JavaScript, at least, it's easy enough to use result types.
If I ever need to mutate a method parameter then I make it a 'ref' so that the people using the method know it will be changed. But, generally, I avoid changing method parameters.
That's why I love that swift forces you to add try before any throwing method, So you always have to handle that or explicitly ignore it with try? But anyone reading the code immediately knows the method throws, And I think extract method is great, but it doesn't exclude the need for a good design and really meaningful method named
I agree with all of this. A) im annoyed if a method is doing too much and you have to navigate several methods deep to figure out what's happening B) a method shouldn't set properties or manipulate an object reference C) you should be able to clearly see the code path from the parent level. I know far too many programmers that try to "make it better" and end up with a rats nest.
IsExistingUserAlreadyActivated can be a spaghetti in itself. So I would be double careful with extracting to bool methods that reach repos under the hood :)
One note. Your new methods accept whole request as argument. But actually uses only some properties. I prefer to pass only required data to the method. It makes the method more reusable and clear from the caller perspective.
Functional core, imperative shell. I want a bunch of small pure static functions that act on immutable data being pipelined through a bunch of well named steps. At the edges of the system, at any point that code could fail for any reason, I want it wrapped in a Result. No global mutable state, no surprises, no inheritance, no race conditions, no need to worry about something being null because having a strong domain model entails that unless all the information is present it cannot create the record in the first place. People think functional is hard. It's not. What's hard is dealing with the combinatorial explosion of chaos and uncertainty that badly done OOP brings with it.
That is why I almost never throw, but use a (secret sauce) return value - so looking forward your mentioned video about that :) Btw, another con of throwing - according to this video - small private method should not throw, but public user call should throw...? That is limiting you a lot - because a method now need to choose a side (throwing / not throwing) - and it can't change sides in the future - because others are counting on it.
I agree, but the example you gave was still reasonable code. However, I do a little smirk when I see a function like (this is TypeScript + React): ``` function updateValue(value: string) { setValue(value); } ``` and I completely die inside when I see a method that, in order to be extracted, had to be passed like 6 arguments, because it's code is completely dependent on the local state of the calling function.
Pure functions(or imperative shell and pure functionals core approach) + immutable variables + anonymous method = the best thing ever for most readable and easy to write code. Anonymous functions solve problem of pure functions when you need pass around too many arguments. Immutable variable allow use variable in multiple anonymous functions and you will not break "purity" of pure functions and decrease amount potential bags.
I somewhat agree with the points you made in this video. But isn't this to some degree also more like an IDE and language issue? An IDE should absolutely be capable of showing that a method might throw, as well as show arguments are being passed as reference. Perhaps even languages should enforce some syntax to indicate such cases, so programmers can at a glance, without any IDE, better understand what's going on.
This video is not about extracting methods, but about side effects. It does not matter how deep down the call stack you decide to throw, you can always say "it's hidden, I need to navigate to it", when looking from the upper layer. Exceptions are part of the interface. Unchecked exceptions mean that every interface should have a warning 'it might blow up'. And you have no clue how and when that happens, until read the 'throw' keyword.
For validation you could extract an IsValid method that just returns a bool and then throw in the caller. I generally try to move validation to the controller/action/endpoint/etc though so by the time it gets to the logic it's already been validated. This can also return ProblemDetails more easily etc
Hey, interesting video! I get the point you're making, it's often about finding a good balance between method extraction and the added indirection it can cause. I liked how it reads when extracting the request validation. For example, validating the existing user could be improved by checking if it's null first, or even better, moving some logic to the user object. I guess the key takeaway from this video is that "Extracting methods isn't a silver bullet for making code more maintainable."
too much scrolling for me in the final version. most of the time I would not care about these validation details. there are some high level tests anyways I assume.
I'd disagree. Refactoring to smaller immutable methods with relevant arguments actually helps in testing, ensuring consistency and reliability. When a large operation receives a large object, validation methods should either operate on the entire object / only broken down properties of the object. So in your case, if validateUsername was a method then it should accept username as a argument and not the entire request. This makes it nimble and relevant. Additionally, refactoring to organize is and optimize readability is helpful as well. It also comes with the responsibility that the child methods should throw exceptions which the parent method will catch and gracefully handle. I advocate for understandable code with small to medium reading context. A medium context fills my screen completely, a small context fills up to half my screen.
Weird timing. I spent the week refactoring a very intricate piece of code and I did a lot of inlining. The code is much easier to follow when it has a more linear flow. I mean, by all means extract consistent units of work, but there’s a lot of benefits to having an easy to follow process instead of 3 or 4 levels of indirection. Also, Linq can be evil and yield return rules. IMO
Programming is a strange thing only engineering discipline where opinions without objective criteria are represented as facts. Your exception point is just silly when every single 'problem ' you mentioned can be solved with renaming the method to indicate a throw in the name, I mean Java has explicit exceptions on methods c# removed it because it's irritating to do so. Usually, people's objections about using exceptions are stupid (except the perf) and focus on some person that can't figure out that has zero reasoning skills and is working on your project. Second point about mutations is also solvable with a name change Calle it RemoveFromTheListChannelsThatAreAgeRestricted, you could even leave out FromTheList but that wouldn't be idiot proof so let's leave it. Also avoiding mutations is an easy way to have performance issues with GC often I seen refactoring like this cause performance issues on hot paths essentially with lists and arrays. For example if the method was join channel that would get called often and you create a new list every time effectively doubling the memory.
What do you do if let's say Sonar is complaining about cognitive complexity? Of couse you could extract some lines into a method, but it feels kinda weird to refactor anything just to satisfy code analyzers.
Agreed the jumping around is a pain, and the changing of the channels is not good... but I have no problem with the validate methods, you dont need to know that its throwing because it is a full exit and there should be no further processing.
So in other words, you have no problem with extract methods. What you have a problem with, is bad coding practices that inexperienced programmers often end up with then doing automated extract methods. Am I understanding you right?
So true, Id call it prematute refactoring. If ur only writing it once, leave it inline, especially when its only 1 or 2 lines. The amount of mental overhead to jump through so many redundant private methods is insane at times. I wrote code before after and during clean code, and the time during and immediately after is some of the worse code I've ever made. The explosion in file size, and if u need to "refactor" to uodate the code, now u have to change return types and parameters everywhere, not just local variables. Thats the real problem with tiny child methods is theyre so painful to change
I strongly agree with you. I’ve coached other engineers towards this same idea. The way I’ve approached talking about this is by starting with why we even extract methods in the first place. It’s just a kind of abstraction. And abstraction is for humans. What does good abstraction give us? The ability to *understand* a piece of code without having to mentally filter out the *distracting* details. You know what’s distracting? Having to jump all around the class to understand the behavior. Keeping the call stack in your head is a mental burden. So we should be extracting methods when the details are so banal and verbose that all they do is distract from the “narrative” of the method.
No. Say you have a method with a bug. The first step is to read the method to find where the bug is likely to be. Lots of abstractions in the narrative can be skipped over because they’re clearly not where the problem lies. When you finally get to the abstraction that looks a likely cause it’s no hardship to jump to the extracted method and delve into the detail. If you have to understand every abstraction along the way you have bigger problems.
@@TonyWhitleyI think we agree. Abstraction has a mental cost. We should only introduce an abstraction when the mental cost is worth it. You might wanna go re-read my comment.
@@hatter1290I don’t agree that abstractions from clean code are mentally challenging. If you have a hundred line method, you’ve still got to process that in your head. I find it much easier to remember segmented code.
@@MiningForPies perhaps I didn’t phrase this as well as I could have. I think you’re talking about your preference for abstraction, which is totally valid. In fact, I think I probably have a similar preference. I have found over large groups of people there is sort of a bell curve of abstraction preference. Again, I think you and I are both on the side of more abstraction is probably better (within reason). However, for those on the other side of the bell curve, they tend to pay a pretty large mental cost for every abstraction. But those who prefer more abstraction are able to comprehend less abstraction with not much more mental effort. And the goal of abstraction should be to maximize the level of *understanding* across the team that works on the code or is likely to work on the code in the near future. If abstraction helps facilitate that understanding, then great! But we should be mindful that abstraction has a variable cost across our team. Abstraction is a tool for facilitating understanding but is not an end in and of itself. The end is the understanding of you and your team.
people extract method for the reason to make the code readable, which I always disagree, since code is written to make a program work and not the code to be read. so the extracting method causes bit of burden on program. so sometimes its better to write linear code where ever possible
That’s OK if the program works perfectly and the requirements never change, only then does no one ever have to read the code and work out what it’s doing and how it’s doing it. Extracting methods rarely causes a *performance* burden, the compiler will optimise it, probably better than we can do; linear, rambling code causes a burden on anyone trying to understand what was in the head of the programmer when they wrote it, which will be me and my code when I look at it a month later!
I am attracted by the idea of one method per file: it’s just as easy (perhaps easier) in an IDE to jump to another tab than elsewhere in the same file (certainly easier to jump back again) and it makes merging easier, particularly if several people are working on the same code. There is a reluctance to have either a module/class/method.ext folder structure or a module.class.method.ext file naming convention but that seems to be just "we're not used to doing that" - source code is strangely antiquated.
People are so into DRY (that's still a thing, right) that they forget the main point of code is to be CLEAR AND UNDERSTANDABLE. Having a lot of conditionals doesn't necessarily mean you need to refactor them to methods; only refactor them if they are unclear. The Zen of Python has it right when it said "Clarity over Cleverness".
I guess you haven't been stuck maintaining a medium to big sized code base with duplicated code all over the place because the original devs thought DRY was just a "deprecated term" or not worth the trade over "clarity"
I'm stuck maintaining a medium to big sized code base with duplicated code all over the place because the original devs had never even *heard* of DRY 🙄
I generally prefer comments to single use extract method. "But comments can get out of date." So do method names. I will make a method if it is worded in the domain - e.g. user.isActivated. But sometimes this doesn't "save" in code space, but rather reifies a domain concept within the codebase.
Depends on the context. Recently i had to extract multiple methods out of a single one cause it was such an unwieldy unreadable mess - turned that thing into i think 15 methods? and it was a looooot better that way. instead of deep nesting and unrelated operations scattered all over the place it now has a nice and simpel to follow structure. No more random checks and returns in the middle of a couple hundred lines of code but a simple: if(!server.getQuery(connection, out var query)) return; if(!querey.loadData(out var data)) return; if(!data.filer(out var filteredData)) return; The code previously of course was littered with comments - cryptic chunks of words that might have meant something to somebody some 15 years ago.
@@ABaumstumpf yes, it always depends. But for a simple validate/act/return style method, it ain't worth it. Methods like this are basically like a unit test. There's rarely a reason to do heavy DRY-ness.
![Megan Thee Stallion - Mamushi (Remix) [feat. TWICE] [Official Audio]](http://i.ytimg.com/vi/5c0_u9PsrsQ/mqdefault.jpg)
I think this video could be abstracted into a separate playlist that should be extracted into a list of playlists. Easy money.
And then I can extract it again into a playlist of playlists of playlists!
Why not use a playlists repository factory of playlists at that point? 🤷♂️
A lazy recursive async playlist factory strategy pattern would make more sense, just to keep it simple.
Would abxtraction work here? 😂
I like the "Clean Code" idea of a method only doing one thing, but in practice it's really annoying to jump around a file, or multiple files, trying to figure out how it all fits together, and identifying the hidden side effects. I think using extract method to create pure functions (ones that don't have side effects, and make use of immutable/read-only data types) is probably a good use-case or guideline to follow for private class methods.
Also, the indirection often means the code's doing unnecessary stuff, like loading the same data from a database twice (I've seen this happen many times).
It always depends. Indirection can be useful for maintenance if done right. Throwing everything in there because "it's easier", makes the code hard to maintain.
@@danflemming3553 I second this. Having a regular sized code base with duplicated code all over the place just to "avoid indirections" is a pain in the ass to maintain. Have a reusable method instead and make your change in only one place
If you are hiding side-effects, you're doing it wrong. Try to use pure functions whenever possible
As with anything, there are some developers who really don't grasp a code concept. You have programmers who happily code a function/method of hundreds of lines of code with global variables/mutables sprinkled all over the place. Others turn 20 lines of code into 60 separate libraries.
I use Extract Method primarily to not to have to write comments. By introducing another method you have an opportunity to give it a meaning full name. That is your 'comment'.
A long method can read like a book, so length is not the primary issue. Of course this is all highly subjective.
Agree. One of the issues with the "50 lines" version is every developer that has to understand what the method essentially has to reverse engineer the method by walking through it the way he did in the video. I don't want to have to read the logic (how it works) to understand the broad concepts (what it does). A section of code will be read many more times than it will be written or modified. Extracted methods save developers time (if done well). Side note, as for the Validate methods that throw, you could name them, e.g., ThrowIfInvalidRequest, etc. to make it obvious.
I was worried by the title, but in the end, I fully agree with your explanations. The problem is not the "Extract Method" refactoring, the problem is extracting the wrong set of lines. E.g. checking a condition and then throwing an exception can already be counted as two separate things, even disregarding that the exception changes the control flow. The condition check could be a useful part of the domain model for reuse elsewhere. The decision what to do based on the outcome could be different in different scenarios. Deep in the code, an exception might be correct, but at the HTTP API level, producing an HTTP 4xx response is probably the better option.
Bonus points if instead of a method, you extract it into a class that now needs a factory to get an instance. For 'extensibility'.
Depends a bit on the complexity of the extracted function, if needed to be extracted anyway. 5 lines 10 line 100 lines,..
throwIfInvalid(user);
gives a fast insight and sums up what it does, no matter how complex. As it not only validates the user, it throws, as part of what it does as well.
This reads just as clear as "if xxz then throw".
But as always, it allways depends! :-)
Yeah, naming is key. If the primary purpose of the method is to cause an exception if some condition fails, maybe include that in the name! Or maybe move your exceptions themselves into another object, move the condition into a bool function, and just do `if (conditionMethod()) throw errors.nameOfError;` or something like that. You don't necessarily need to extract the entire logic of what you are doing, but you can make the code a lot cleaner and more manageable by putting the complicated or verbose portions someplace else.
Call the method "throwIf..." and you'll know it's throwing.
+1 for returning a modified collection rather than modifying the passed argument.
It is also a common pattern in Java to use void functions that start with "validate" and it is very clear (even though it's still only implied) that it throws if there's a validation error. eg. void validateJwt(String token)
@@essamal-mansouri2689 In Java, you can use the `throws` keyword though... But yeah, `validateSth` is quite a widespread convention.
It is all about context. This method is involved with creating a user. The moment channels becomes the main subject, the code goes into a new method.
The existing user could have stayed contextwise, but readability is another factor to consider.
I like to abstract even further by creating smaller "classes" to handle the tasks like "Validation", "Database Operations" and other logic. Extract Method still has a good place, like in the video's example, where it makes complex conditional statements extremely read-able in the actual method itself. Additionally, the smaller classes make it real easy to test that code in isolation from the rest of the context and logic of the program.
Also having so many layers, and so many behaviors for that matter inside a methohd, makes the unit test a real pain
This is why I'm such a fan of FP: I want those bad practices to be impossible, guaranteed by the compiler.
Yeah. I believe any kind of Refactoring should be considered as a trade-off rather than a value-added action. That's why Refactoring techniques usually come in pair, so that when we find one is becoming too much, we can re-calibrate the trade-off with the opposite one (which is the later part of your video).
For the case "Extract Method", its pair would be "Inline Method." But the problem we have at the moment is most IDEs offer the automatic "Extract Method" refactoring, but not the automatic "Inline Method" refactoring. So it becomes too easy to extract methods automatically, but only a few who properly learn the craft from Martin Fowler and Kent Beck's book can safely inline them back manually.
And the way IDEs only offer automatic "Extract Method" may also give many developers the wrong impression that "Extract Method" is a good thing and should be encouraged. So... we have "Extract Method" happening and spreading like cancer.
50 lines with little nesting? Very fine in my books, usually.
Hundreds of lines, thousands? With deep nesting? I dunno how many such methods I've encountered , a lot certainly, "Extract Method" was the first shortcut command I learned in VS. Just the other day I was working on a class that had the same calculation duplicated 19 times in one method.
Usually I will only have to do "Inline Method" in my own code where I use "Extract Method" as a way of exploration.
I think the Validate method is solved by having conventions.
For example we use requireXX. When reading the require prefix and by being (usually) at top of the method I think it's pretty okay to know it throws.
Ofc here the validations are trivial and don't really need to be extracted anyway as you pointed out.
Other than this I agree 💯%.
Primarily, extracting code blocks are actually very useful in 2 scenarios for me
1. When it becomes easy for me to lose track of the execution sequence of a code. I prefer to group each block by functionality. That way, I'm able to follow my logic while not worrying about the implementation of the high level lines of code
2. When a block of code is being reused across multiple implementations.
Are you supposed to read the code and learn everything how it works? I don't think so. If you need to learn how everything works because there's a bug which is hard to pinpoint, that kind of bug is hard to fix if the code is bad even without all the indirection by different methods. In your example, bad design like the FilterAgeRestrictedChannels which does not follow CQS, is different and not related to the indirection. Bad indirection does exist, but I don't think your example was a good one. The ValidateUsername and ValidateExistingUser were OK, when I want to extend the code in the Handle(), I want to understand it quickly.
explain "FilterAgeRestrictedChannels which does not follow CQS"
@@Ry4nWTF It's what Derek shows in the video, his refactoring is good, it's following the functional style
@@Ry4nWTF CQS - command query segregation, meaning a method should either return a result (query) or perform state changes (command), but not both. Bascially, you should avoid modifying your input params.
In the `FilterAgeRestrictedChannels` method, he was modifying the `channel` input with the `addRange` call. Hence, breaking the CQS principle.
This is a really bad code smell in my book, as I have been bitten by bugs introduced by such practices in the past, and it's really hard to debug. This is also why FP fellas mock OOP (mutable states all over the place), and what functional features like LINQ are good at solving. (Dang I miss LINQ so much. Java Streams are so verbose...)
A method should have one level of abstraction. If the method has one level of abstraction then it is okay that it is contain many lines because it's is easy to read.
I would say that as a rule of thumb for non-public code: Only extract when you need to hide parts of the implementation, or rather behavior, behind a name that makes sense. Developer are bad at doing in consistently when only want to finishing things. This might cause more convoluted code with names not indicating the behavior. So they think reviews will catch those, not necessarily since people are looking more at style. Since no one discusses the actual design, in my experience.
I totally agree with this. It's annoying when people want to extract method just because something is long, and then end up passing a million parameters and naming the methods with nonsensical names. Btw, I wouldn't call this "indirection"; I use that word for calling abstract methods (which also really hurts readability in general).
Are you criticizing "composed method". Your refactoring is just wrong. You could do it so everything is obvious.
While also being kind of an abstraction, I like to define the conditionals as variables if I want to give them a name, that way the logic stays in scope
That is the way to work with code, loved watching it.
Basically the attempt to extract meaningful code blocks from large methods is very valuable. But yes, if not done carefully, it can increase the "spaghettiness" a lot, while actually wanting to improve the code.
It is worth reasoning about what readability means for that code at hand. It is not the formatting alone of course. But even when the "good" refactorings are not easy to spot, it is important to keep doing this continuously, even when the code seems already finished.
The more skilled at refactoring we are, the better code quality becomes, both of existing and of new code. I personally think that refactoring is the most underrated but most important activity in coding.
And we see how important naming is. And immutability, or that a method should either check state or change it, but never do both at the same time.
Sometimes I find myself inlining short methods back in. And then sometimes it makes sense to extract a class and/or interface. We should not overlook that option, while this one must be handled with even a lot more care 🫣😄
Looking forward to that video about exceptions versus return values / control flow! 😃 Great stuff 👍
This seems like the problem is doing side effects in methods and not doing extract method. If your method was profile.Channels = RestrictChannels(profile.Channels) then it would have been very clear. Same with the result type which you already mentioned. An extra layer of indirection is fine if it helps you read the code at a glance IMO. It can be taken too far, but code that reads like natural language is very easy to figure out what it is (or should be) doing. Your tests will hopefully make sure that the actual implementation is fine. Like you said, it depends on the context, but I don't hate the advice of trying to stick to one layer of abstraction per method.
Absolutely it's side-effects as well as levels of indirection. My example wasn't so bad but I think we've all read or created without realizing it some absurd levels of indirection
I didn't even know Extract Method was even a thing. I always manually do it, and I usually avoid mutating parameters where I can. Validation methods that throw errors or just return usually have a naming convention that I follow, but admittedly, it's probably better to just return a [error, result] type. If only I could do that in Java a lot easier without tons of boilerplate. I don't know if a good solution has emerged beyond Java 8. In JavaScript, at least, it's easy enough to use result types.
If I ever need to mutate a method parameter then I make it a 'ref' so that the people using the method know it will be changed. But, generally, I avoid changing method parameters.
That's why I love that swift forces you to add try before any throwing method,
So you always have to handle that or explicitly ignore it with try? But anyone reading the code immediately knows the method throws,
And I think extract method is great, but it doesn't exclude the need for a good design and really meaningful method named
I agree with all of this.
A) im annoyed if a method is doing too much and you have to navigate several methods deep to figure out what's happening
B) a method shouldn't set properties or manipulate an object reference
C) you should be able to clearly see the code path from the parent level. I know far too many programmers that try to "make it better" and end up with a rats nest.
always be modest and only refactor if there are some benefit ( like extracting method to another class and use it in multiple places )
IsExistingUserAlreadyActivated can be a spaghetti in itself. So I would be double careful with extracting to bool methods that reach repos under the hood :)
One note. Your new methods accept whole request as argument. But actually uses only some properties. I prefer to pass only required data to the method. It makes the method more reusable and clear from the caller perspective.
Functional core, imperative shell. I want a bunch of small pure static functions that act on immutable data being pipelined through a bunch of well named steps. At the edges of the system, at any point that code could fail for any reason, I want it wrapped in a Result. No global mutable state, no surprises, no inheritance, no race conditions, no need to worry about something being null because having a strong domain model entails that unless all the information is present it cannot create the record in the first place.
People think functional is hard. It's not. What's hard is dealing with the combinatorial explosion of chaos and uncertainty that badly done OOP brings with it.
That is why I almost never throw, but use a (secret sauce) return value - so looking forward your mentioned video about that :)
Btw, another con of throwing - according to this video - small private method should not throw, but public user call should throw...?
That is limiting you a lot - because a method now need to choose a side (throwing / not throwing) - and it can't change sides in the future - because others are counting on it.
I agree, but the example you gave was still reasonable code. However, I do a little smirk when I see a function like (this is TypeScript + React):
```
function updateValue(value: string) {
setValue(value);
}
```
and I completely die inside when I see a method that, in order to be extracted, had to be passed like 6 arguments, because it's code is completely dependent on the local state of the calling function.
Pure functions(or imperative shell and pure functionals core approach) + immutable variables + anonymous method = the best thing ever for most readable and easy to write code. Anonymous functions solve problem of pure functions when you need pass around too many arguments. Immutable variable allow use variable in multiple anonymous functions and you will not break "purity" of pure functions and decrease amount potential bags.
I somewhat agree with the points you made in this video. But isn't this to some degree also more like an IDE and language issue? An IDE should absolutely be capable of showing that a method might throw, as well as show arguments are being passed as reference. Perhaps even languages should enforce some syntax to indicate such cases, so programmers can at a glance, without any IDE, better understand what's going on.
This video is not about extracting methods, but about side effects.
It does not matter how deep down the call stack you decide to throw, you can always say "it's hidden, I need to navigate to it", when looking from the upper layer.
Exceptions are part of the interface.
Unchecked exceptions mean that every interface should have a warning 'it might blow up'. And you have no clue how and when that happens, until read the 'throw' keyword.
I like long readable methods vs small drill drill methods
I don't like going super crazy with extracting methods, unless for DRY reasons or when dealing with medium to complex conditionals.
For validation you could extract an IsValid method that just returns a bool and then throw in the caller.
I generally try to move validation to the controller/action/endpoint/etc though so by the time it gets to the logic it's already been validated. This can also return ProblemDetails more easily etc
Hey, interesting video! I get the point you're making, it's often about finding a good balance between method extraction and the added indirection it can cause.
I liked how it reads when extracting the request validation. For example, validating the existing user could be improved by checking if it's null first, or even better, moving some logic to the user object.
I guess the key takeaway from this video is that "Extracting methods isn't a silver bullet for making code more maintainable."
Ya, it's got it's caveats ultimately.
too much scrolling for me in the final version. most of the time I would not care about these validation details. there are some high level tests anyways I assume.
I'd disagree. Refactoring to smaller immutable methods with relevant arguments actually helps in testing, ensuring consistency and reliability. When a large operation receives a large object, validation methods should either operate on the entire object / only broken down properties of the object. So in your case, if validateUsername was a method then it should accept username as a argument and not the entire request. This makes it nimble and relevant. Additionally, refactoring to organize is and optimize readability is helpful as well. It also comes with the responsibility that the child methods should throw exceptions which the parent method will catch and gracefully handle. I advocate for understandable code with small to medium reading context. A medium context fills my screen completely, a small context fills up to half my screen.
It’s not indirection it’s readability that is also running code
Weird timing. I spent the week refactoring a very intricate piece of code and I did a lot of inlining. The code is much easier to follow when it has a more linear flow. I mean, by all means extract consistent units of work, but there’s a lot of benefits to having an easy to follow process instead of 3 or 4 levels of indirection. Also, Linq can be evil and yield return rules. IMO
I agree. All about balance.
Dislikes are probably coming from Uncle Bob :). Great video, sometimes clean code patterns are useful, but other times they are simply not.
Ha.
FluentValidation?
Programming is a strange thing only engineering discipline where opinions without objective criteria are represented as facts. Your exception point is just silly when every single 'problem ' you mentioned can be solved with renaming the method to indicate a throw in the name, I mean Java has explicit exceptions on methods c# removed it because it's irritating to do so. Usually, people's objections about using exceptions are stupid (except the perf) and focus on some person that can't figure out that has zero reasoning skills and is working on your project. Second point about mutations is also solvable with a name change Calle it RemoveFromTheListChannelsThatAreAgeRestricted, you could even leave out FromTheList but that wouldn't be idiot proof so let's leave it. Also avoiding mutations is an easy way to have performance issues with GC often I seen refactoring like this cause performance issues on hot paths essentially with lists and arrays. For example if the method was join channel that would get called often and you create a new list every time effectively doubling the memory.
If I need to read all your code in order to understand what it is doing you’re doing something wrong
Martin Fowler, please relogin
What do you do if let's say Sonar is complaining about cognitive complexity? Of couse you could extract some lines into a method, but it feels kinda weird to refactor anything just to satisfy code analyzers.
I think they can good indicators but not full proof of what you really want.
What Outbox library are you using?
None, it was just an example nothing really behind the scenes.
Agreed the jumping around is a pain, and the changing of the channels is not good... but I have no problem with the validate methods, you dont need to know that its throwing because it is a full exit and there should be no further processing.
refactoring
So basically you are saying, use your head and don't rely on a tool/feature.
Easier said than done.
Great Video - As usual :)
So in other words, you have no problem with extract methods. What you have a problem with, is bad coding practices that inexperienced programmers often end up with then doing automated extract methods. Am I understanding you right?
Yes. Extract method isn't inherently bad, of course.
Thanks for the video :)
Sure thing!
Extract
So true, Id call it prematute refactoring. If ur only writing it once, leave it inline, especially when its only 1 or 2 lines. The amount of mental overhead to jump through so many redundant private methods is insane at times. I wrote code before after and during clean code, and the time during and immediately after is some of the worse code I've ever made. The explosion in file size, and if u need to "refactor" to uodate the code, now u have to change return types and parameters everywhere, not just local variables. Thats the real problem with tiny child methods is theyre so painful to change
This violates clean code principles of Robert C. Martin 🙂
Sure
I strongly agree with you. I’ve coached other engineers towards this same idea. The way I’ve approached talking about this is by starting with why we even extract methods in the first place. It’s just a kind of abstraction. And abstraction is for humans. What does good abstraction give us? The ability to *understand* a piece of code without having to mentally filter out the *distracting* details. You know what’s distracting? Having to jump all around the class to understand the behavior. Keeping the call stack in your head is a mental burden. So we should be extracting methods when the details are so banal and verbose that all they do is distract from the “narrative” of the method.
No. Say you have a method with a bug. The first step is to read the method to find where the bug is likely to be. Lots of abstractions in the narrative can be skipped over because they’re clearly not where the problem lies. When you finally get to the abstraction that looks a likely cause it’s no hardship to jump to the extracted method and delve into the detail.
If you have to understand every abstraction along the way you have bigger problems.
@@TonyWhitleyI think we agree. Abstraction has a mental cost. We should only introduce an abstraction when the mental cost is worth it. You might wanna go re-read my comment.
@@hatter1290I don’t agree that abstractions from clean code are mentally challenging. If you have a hundred line method, you’ve still got to process that in your head. I find it much easier to remember segmented code.
@@MiningForPies perhaps I didn’t phrase this as well as I could have. I think you’re talking about your preference for abstraction, which is totally valid. In fact, I think I probably have a similar preference. I have found over large groups of people there is sort of a bell curve of abstraction preference. Again, I think you and I are both on the side of more abstraction is probably better (within reason). However, for those on the other side of the bell curve, they tend to pay a pretty large mental cost for every abstraction. But those who prefer more abstraction are able to comprehend less abstraction with not much more mental effort. And the goal of abstraction should be to maximize the level of *understanding* across the team that works on the code or is likely to work on the code in the near future. If abstraction helps facilitate that understanding, then great! But we should be mindful that abstraction has a variable cost across our team. Abstraction is a tool for facilitating understanding but is not an end in and of itself. The end is the understanding of you and your team.
hard to read
method
people extract method for the reason to make the code readable, which I always disagree, since code is written to make a program work and not the code to be read. so the extracting method causes bit of burden on program. so sometimes its better to write linear code where ever possible
That’s OK if the program works perfectly and the requirements never change, only then does no one ever have to read the code and work out what it’s doing and how it’s doing it. Extracting methods rarely causes a *performance* burden, the compiler will optimise it, probably better than we can do; linear, rambling code causes a burden on anyone trying to understand what was in the head of the programmer when they wrote it, which will be me and my code when I look at it a month later!
I am attracted by the idea of one method per file: it’s just as easy (perhaps easier) in an IDE to jump to another tab than elsewhere in the same file (certainly easier to jump back again) and it makes merging easier, particularly if several people are working on the same code. There is a reluctance to have either a module/class/method.ext folder structure or a module.class.method.ext file naming convention but that seems to be just "we're not used to doing that" - source code is strangely antiquated.
can be
People are so into DRY (that's still a thing, right) that they forget the main point of code is to be CLEAR AND UNDERSTANDABLE. Having a lot of conditionals doesn't necessarily mean you need to refactor them to methods; only refactor them if they are unclear. The Zen of Python has it right when it said "Clarity over Cleverness".
I guess you haven't been stuck maintaining a medium to big sized code base with duplicated code all over the place because the original devs thought DRY was just a "deprecated term" or not worth the trade over "clarity"
I'm stuck maintaining a medium to big sized code base with duplicated code all over the place because the original devs had never even *heard* of DRY 🙄
This whole video feels powered by a naïve notion of coding over engineering. Sad
What I'm illustrating isn't uncommon at all.
@@CodeOpinion true, I see it in novices often until they learn to write code that describes its run state
I generally prefer comments to single use extract method. "But comments can get out of date." So do method names.
I will make a method if it is worded in the domain - e.g. user.isActivated. But sometimes this doesn't "save" in code space, but rather reifies a domain concept within the codebase.
Depends on the context.
Recently i had to extract multiple methods out of a single one cause it was such an unwieldy unreadable mess - turned that thing into i think 15 methods? and it was a looooot better that way. instead of deep nesting and unrelated operations scattered all over the place it now has a nice and simpel to follow structure. No more random checks and returns in the middle of a couple hundred lines of code but a simple:
if(!server.getQuery(connection, out var query)) return;
if(!querey.loadData(out var data)) return;
if(!data.filer(out var filteredData)) return;
The code previously of course was littered with comments - cryptic chunks of words that might have meant something to somebody some 15 years ago.
Code that OP produces would generally be un unreadable overtime
@@ABaumstumpf yes, it always depends. But for a simple validate/act/return style method, it ain't worth it.
Methods like this are basically like a unit test. There's rarely a reason to do heavy DRY-ness.